Skip to content

fix(tlsmiddlebox): make ClientId settable and validate its value - #1811

Open
munzzyy wants to merge 1 commit into
ooni:masterfrom
munzzyy:fix/tlsmiddlebox-clientid
Open

fix(tlsmiddlebox): make ClientId settable and validate its value#1811
munzzyy wants to merge 1 commit into
ooni:masterfrom
munzzyy:fix/tlsmiddlebox-clientid

Conversation

@munzzyy

@munzzyy munzzyy commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Checklist

  • I have read the contribution guidelines
  • reference issue for this pull request: none, found this while poking at experiment options
  • if you changed anything related to how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: N/A, no data format change
  • if you changed code inside an experiment, make sure you bump its version number: 0.1.2 to 0.1.3

Description

tlsmiddlebox declares its ClientId option as int, but SetOptionAny in internal/registry only knows how to set int64, bool, and string fields. So the option is documented (it has an ooni tag and shows up in the options list) but you cannot actually use it. Applying the registry test this PR adds on top of current master:

$ go test -count=1 -run 'TestExperimentOptionsAreAlwaysSettable' ./internal/registry/
--- FAIL: TestExperimentOptionsAreAlwaysSettable (0.00s)
    --- FAIL: TestExperimentOptionsAreAlwaysSettable/tlsmiddlebox (0.00s)
        factory_test.go:1138: field ClientId has kind int, which SetOptionAny cannot set

miniooni tlsmiddlebox -O ClientId=2 is where that bites. miniooni passes option values as strings, string values for integer options go through setOptionInt, and SetOptionAny only routes to setOptionInt for fields whose kind is int64. An int field falls through to the default branch instead, which returns unsupported option type: string. Every other experiment already declares integer options as int64; tlsmiddlebox was the only one using plain int.

There is a second problem behind the first one. OONI Run v2 descriptors apply options with SetOptionsJSON, which unmarshals straight into the config struct and bypasses the kind check, so a descriptor can set any ClientId it likes. ClientIDs in tracing.go is a map[int]*utls.ClientHelloID with keys 1 to 4, the lookup silently returns nil for any other key, and netxlite.NewUTLSConn dereferences the pointer. Running the measurer with Config{ClientId: 5} against a local TLS listener on current master:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xb5ca41]

github.com/ooni/probe-cli/v3/internal/netxlite.NewUTLSConn(...)
	internal/netxlite/utls.go:83
...
github.com/ooni/probe-cli/v3/internal/experiment/tlsmiddlebox.(*Measurer).handshakeWithTTL(...)
	internal/experiment/tlsmiddlebox/tracing.go:100

The fix is small: ClientId becomes int64 (with clientid() casting for the map lookup, so tracing.go is untouched), and Run now rejects a ClientId that does not match any known fingerprint, the same way it already rejects an invalid test helper. I picked an error over a silent fallback to the stdlib handshaker because somebody who asked for a Chrome fingerprint should not quietly measure with the wrong ClientHello.

The registry test shown above walks every experiment config and flags any ooni-tagged field whose kind SetOptionAny cannot set, so a future experiment cannot reintroduce this class of bug.

Testing

$ go test -count=1 -race ./internal/experiment/tlsmiddlebox/ ./internal/registry/
ok  	github.com/ooni/probe-cli/v3/internal/experiment/tlsmiddlebox	61.851s
ok  	github.com/ooni/probe-cli/v3/internal/registry	1.084s

New coverage: the invalid ClientId path in Run, the non-default clientid() value, and the registry-wide option kind check. go vet and gofmt are clean on both packages.

The ClientId option was declared as int, but SetOptionAny only knows
how to set int64, bool, and string fields, so configuring the option
from the command line never worked. Applying the registry test this
commit adds on top of its parent shows it:

    $ go test -count=1 -run 'TestExperimentOptionsAreAlwaysSettable' ./internal/registry/
    --- FAIL: TestExperimentOptionsAreAlwaysSettable (0.00s)
        --- FAIL: TestExperimentOptionsAreAlwaysSettable/tlsmiddlebox (0.00s)
            factory_test.go:1138: field ClientId has kind int, which SetOptionAny cannot set

Every other experiment already declares integer options as int64, so
this switches the field type and keeps clientid() returning int for
the ClientIDs map lookup.

Setting ClientId was still possible through OONI Run v2 descriptors,
whose options bypass SetOptionAny via SetOptionsJSON, and any value
outside the known 1-4 range crashed the process: ClientIDs is a map,
so the lookup silently returns nil for unknown keys and
netxlite.NewUTLSConn dereferences the nil ClientHelloID:

    panic: runtime error: invalid memory address or nil pointer dereference
    github.com/ooni/probe-cli/v3/internal/netxlite.NewUTLSConn
        internal/netxlite/utls.go:83
    github.com/ooni/probe-cli/v3/internal/experiment/tlsmiddlebox.(*Measurer).handshakeWithTTL
        internal/experiment/tlsmiddlebox/tracing.go:100

Run now rejects unknown ClientId values upfront, like we already do
for other invalid configuration, and the new registry test makes sure
every option declared with an ooni tag uses a kind that SetOptionAny
can actually set.
@munzzyy
munzzyy requested review from DecFox and hellais as code owners August 3, 2026 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant